Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encourage NixOS configuration, nix-shell and discourage nix-env #514

Merged
merged 3 commits into from
Aug 11, 2022

Conversation

MatthewCroughan
Copy link
Contributor

@MatthewCroughan MatthewCroughan commented Aug 6, 2022

This would close #508

High level changes

  1. nix-env now has a warning associated with it, which explains that it has side-effects which may require cleanup by the user manually after usage like a traditional package manager.
  2. NixOS configuration(s) and nix-shell have now got a tab on each package, which explains how to use environment.systemPackages or nix-shell -p.

Internal changes

  1. I have replaced FromNixOS and FromNixpkgs with ViaNixOS, ViaNixShell and ViaNixEnv
  2. I have removed the $ before every instance of .terminal in frontend/src/index.less because I was not easily able to case split on imperative cmds like nix-shell -p and the code for a NixOS configuration. Instead, $ has been placed manually into each text field where a command is imperative, such as nix build for flakes and nix-shell.
  3. I'm no ELM expert, so have likely made wrong use of the div function, and would appreciate someone to help with that

What it looks like

NixOS Configuration

image

nix-shell

image

nix-env

image

Copy link
Member

@Kranzes Kranzes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has been approved by the IANEO, the International Anti-nix-env Organization. Consider your work worthy. ✅

Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright.

Let's maybe move the "long description" above the installation instructions?

Ping @roberth

Comment on lines -10 to -12
&:before {
content: "$ "
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of having this in a :before is that the dollar sign is not copied when you select the whole block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pre-existing css is the strangest I've ever seen. What's up with all the nth-child selectors? Positional as opposed to named css? That's just an awful idea.
So I can't blame Matthew for not restoring this functionality for nix-env. He had to remove the $ for the declarative installation methods, which are more important anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, it's awful. There's no need to stick to that pattern though: adding a shell-commands class outside the hierarchy would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with elm enough to accomplish this. If you could make a PR that does it to my fork, I'll merge it and it'll be done. But I think the scope of this PR will then have been increased more than it needs to be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! I pushed to your branch directly.

@roberth
Copy link
Member

roberth commented Aug 7, 2022

An example with a shell file would be nice, but certainly no blocker.

@MatthewCroughan
Copy link
Contributor Author

@ncfavier Having reviewed your changes, it looks great.

image

Copy link

@fufexan fufexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff :)

@roberth
Copy link
Member

roberth commented Aug 11, 2022

@garbas This looks ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

packages: Offer more installation method suggestions and discourage nix-env -i
6 participants